Skip to content

Bench vs other vms#365

Merged
nicole-graus merged 26 commits into
mainfrom
bench_vs
Apr 20, 2026
Merged

Bench vs other vms#365
nicole-graus merged 26 commits into
mainfrom
bench_vs

Conversation

@MauroToscano

@MauroToscano MauroToscano commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actions Bot commented Feb 25, 2026

Copy link
Copy Markdown

Codex Code Review

Findings

  1. Potential bug (Medium): fragile parsing causes hard failure under set -euo pipefail
    bench_vs/run.sh uses command substitutions with grep -o to extract times and cycles. If the output format ever changes or is empty, grep exits 1 and the script terminates due to set -e/pipefail, aborting the entire benchmark run. This is likely in CI or user environments where the CLI output differs.
    File: bench_vs/run.sh

  2. Potential bug (Low): divide-by-empty/awk error if parsing fails
    The summary ratio computation assumes numeric lt and st. If either is empty (parsing failure) but not "n/a", the awk expression becomes invalid and the script exits. This is an immediate abort rather than a graceful “n/a”.
    File: bench_vs/run.sh

  3. Security hygiene (Low): committed local .tldr state
    bench_vs/sp1/fibonacci/.tldr/daemon.pid and bench_vs/sp1/fibonacci/.tldr/status look like local runtime artifacts. While not a direct vulnerability, committing daemon PID/state can leak environment details and is generally unsafe to keep in repo.
    Files: bench_vs/sp1/fibonacci/.tldr/daemon.pid, bench_vs/sp1/fibonacci/.tldr/status

No critical or high-severity issues found.

If you want, I can propose small fixes for the parsing robustness and cleanup of .tldr artifacts.

@claude

claude Bot commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Review: Bench vs other VMs

Bug (Medium) — Missing bench_vs/lambda/ directory

run.sh sets LAMBDA_DIR="$SCRIPT_DIR/lambda/fibonacci" (line 74) but no bench_vs/lambda/fibonacci directory exists in this PR. Running without --sp1-only will immediately fail at the cd in run_one (line 94) because set -euo pipefail propagates into the subshell. The Lambda VM half of the benchmark is effectively broken as merged.

Accidental commits — .tldr/ files should not be in the repo

Three files from what appears to be a local AI dev-tool (.tldr) were committed:

  • bench_vs/sp1/fibonacci/.tldr/daemon.pid — contains hardcoded local PID 39495
  • bench_vs/sp1/fibonacci/.tldr/status — contains ready
  • bench_vs/sp1/fibonacci/.tldrignore — auto-generated local ignore config

None of these belong in version control. They should be removed and added to .gitignore.

claude[bot]
claude Bot reviewed Feb 25, 2026
View reviewed changes
Comment thread bench_vs/run.sh
claude[bot]
claude Bot reviewed Feb 25, 2026
View reviewed changes
Comment thread bench_vs/sp1/fibonacci/.tldr/daemon.pid Outdated
gabrielbosio
gabrielbosio requested changes Feb 26, 2026
View reviewed changes
Comment thread bench_vs/run.sh Outdated
gabrielbosio
gabrielbosio previously approved these changes Feb 27, 2026
View reviewed changes
@MauroToscano @gabrielbosio
Apply suggestion from @gabrielbosio
0069659 
Co-authored-by: Gabriel Bosio <38794644+gabrielbosio@users.noreply.github.com>
@github-actions

github-actions Bot commented Mar 4, 2026

Copy link
Copy Markdown

Benchmark Results for unmodified programs 🚀

Command Mean [ms] Min [ms] Max [ms] Relative
base binary_search 55.9 ± 0.8 54.7 57.1 1.00
head binary_search 57.6 ± 8.7 54.0 82.3 1.03 ± 0.16
Command Mean [ms] Min [ms] Max [ms] Relative
base bitwise_ops 55.6 ± 1.4 53.1 57.3 1.00
head bitwise_ops 60.8 ± 16.9 53.6 108.9 1.09 ± 0.31
Command Mean [ms] Min [ms] Max [ms] Relative
base fibonacci_26 61.3 ± 3.1 59.1 69.5 1.00
head fibonacci_26 62.8 ± 4.4 59.2 72.8 1.02 ± 0.09
Command Mean [ms] Min [ms] Max [ms] Relative
base hashmap 114.8 ± 1.9 111.6 118.2 1.00
head hashmap 118.0 ± 5.9 114.1 134.2 1.03 ± 0.05
Command Mean [ms] Min [ms] Max [ms] Relative
base keccak 121.4 ± 3.7 119.1 130.8 1.01 ± 0.05
head keccak 120.5 ± 5.0 116.3 133.3 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base matrix_multiply 68.4 ± 1.0 67.0 70.4 1.00
head matrix_multiply 69.2 ± 1.6 67.9 73.0 1.01 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base modular_exp 55.3 ± 1.0 54.0 57.2 1.00
head modular_exp 59.4 ± 3.1 55.8 66.0 1.07 ± 0.06
Command Mean [ms] Min [ms] Max [ms] Relative
base quicksort 59.0 ± 1.3 56.8 61.8 1.00 ± 0.03
head quicksort 58.9 ± 1.5 56.5 61.8 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base sieve 62.2 ± 1.5 60.2 65.5 1.00 ± 0.03
head sieve 62.2 ± 0.8 60.4 63.3 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base sum_array 72.2 ± 1.2 70.3 74.2 1.00
head sum_array 72.8 ± 1.7 70.7 75.2 1.01 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base syscall_commit 87.2 ± 1.5 84.6 89.8 1.00
head syscall_commit 87.4 ± 1.9 85.0 91.5 1.00 ± 0.03

@nicole-graus
nicole-graus enabled auto-merge April 20, 2026 14:44
@nicole-graus
nicole-graus added this pull request to the merge queue Apr 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 20, 2026
@nicole-graus
nicole-graus added this pull request to the merge queue Apr 20, 2026
Merged via the queue into main with commit 0b0e6d3 Apr 20, 2026
10 checks passed
@nicole-graus
nicole-graus deleted the bench_vs branch April 20, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments
@gabrielbosio gabrielbosio gabrielbosio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MauroToscano @gabrielbosio @nicole-graus @jotabulacios